[Do not merge] Iterative bind with a stack instead of recursion#1783
[Do not merge] Iterative bind with a stack instead of recursion#1783smaheshwar-pltr wants to merge 1 commit intoapache:mainfrom
bind with a stack instead of recursion#1783Conversation
smaheshwar-pltr
changed the title
bind with a stack instead of recursionbind with a stack instead of recursion
| assert upd.rows_inserted == 4 | ||
|
|
||
|
|
||
| def test_large_upsert_into_empty_table(catalog: Catalog) -> None: |
There was a problem hiding this comment.
This now-passing test fails on main with
def __getitem__(self, key):
> return self.data[ref(key)]
E RecursionError: maximum recursion depth exceeded in comparison
/<PATH>/weakref.py:416: RecursionError
!!! Recursion error detected, but an error occurred locating the origin of recursion.
The following exception happened when comparing locals in the stack frame:
RecursionError: maximum recursion depth exceeded
Displaying first and last 10 stack frames out of 962.
| num_columns = 50 | ||
| num_rows = 10000 |
There was a problem hiding this comment.
Actually, 20 and 1000 is enough to make main fail this test with (default) recursion depth exceeded
|
changing the visitor to an iterative approach seems like a sound solution. are there any reasons we dont want to do this? |
|
I like the solution!
My only concern is performance. We probably want to check what the impact is, since we bind to schemas all over the codebase. It would be good to see if we can get some numbers on the impact on the performance. |
|
Thanks for taking a look folks, and apologies for the delayed response.
This approach serves only to circumvent recursion. When it's said that "iteration is faster than recursion", I don't think it refers to just concretising the frames / call-stack in memory - I believe this aligns with @Fokko mentioning that performance might be worsened, which I agree with. If we do want to go with this PR's approach, then I think we should consider it for the other visitors in I see some recent activity / PRs on the original issue so happy to wait for that discussion to conclude? |
3 participants
// Do not merge.
For fun, writing out the call stack to avoid the recursion that caused #1759.